Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Practical fix for securedrop-client#1025 Faster sync for all source #5204

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Apr 22, 2020

Status

Ready for review

Description of Changes

Fixes freedomofpress/securedrop-client#1025

Reimplements the get_sources call. On a staging system, direct calls using exposed port gave us the following numbers. Key size: 1024, all keys and fingerprints are in redis cache.

1461 sources, 50 submissions, and 20 replies for each.

This is the pure python implementation (as in this PR).

1.61 s ± 38.5 ms per loop (mean ± std. dev. of 3 runs, 50 loops each)

I also tested the same logic in a complied Rust extension just to see the difference.

Python module written in Rust on mod_wsgi-express

1.11 s ± 47.3 ms per loop (mean ± std. dev. of 3 runs, 50 loops each)

Python module written in Rust on our apache2

1.44 s ± 114 ms per loop (mean ± std. dev. of 3 runs, 50 loops each)

Testing

How should the reviewer test this PR?

  • make staging
  • Run the client against this server.
  • CI is green

I am waiting for the CI :)

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing production instances.
  2. New installs.

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make test) pass in the development container

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

If you made changes to documentation:

  • Doc linting (make docs-lint) passed locally

If you added or updated a code dependency:

Choose one of the following:

  • I have performed a diff review and pasted the contents to the packaging wiki
  • I would like someone else to do the diff review

Implements only get_sources call
@eloquence
Copy link
Member

@rmol @kushaldas @creviera Is this worth revisiting, or should we close it?

@kushaldas
Copy link
Contributor Author

If v2 API is further away time line wise, I think we should look at this one.

@eloquence eloquence changed the title Partical fix for #1025 Faster sync for all source Practical fix for #1025 Faster sync for all source Sep 4, 2020
@cfm cfm changed the title Practical fix for #1025 Faster sync for all source Practical fix for securedrop-client#1025 Faster sync for all source Nov 22, 2022
@cfm cfm added needs/discussion queued up for discussion at future team meeting. Use judiciously. api labels Nov 22, 2022
@cfm
Copy link
Member

cfm commented Nov 22, 2022

I've updated the title and description here to reflect that it addresses freedomofpress/securedrop-client#1025, not #1025 here. I think we should revisit this in the broader conversation around when and how to consider the next version of the Journalist API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api needs/discussion queued up for discussion at future team meeting. Use judiciously.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is 40 seconds still a good default timeout for a sync?
3 participants